Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ux 431 static testing #34

Merged
merged 12 commits into from
Feb 12, 2019
Merged

Ux 431 static testing #34

merged 12 commits into from
Feb 12, 2019

Conversation

nahumzs
Copy link
Contributor

@nahumzs nahumzs commented Feb 1, 2019

Resolved #2

🛠 Purpose

  1. Fixing all eslint issues and prettify our code.
  2. Setup tools and config that allowed to do the previous job seamlessly.
  3. Make use of husky so can validate all the previous before committing code
  4. Adding basic PR Github Template

Setting up different static testing tools.

  • eslint
  • prettier
  • yarn audit
  • husky 🐶
  • yarn audit

Version requires to make the previous tools to work

  • node 9.10.0 a nvm file has been added
  • yarn 1.16.3 added engine entry in package.json has been added `yarn: ">= 16.3.0" warning of the requirement, this is need to use yarn audit.

Husky hooks

Now every time a dev want to commit something the following will occur:

  1. eslint
  2. jest
  3. jest a11y
  4. check for prettier consistency
  5. we could add yarn audit but I'm not sure about it.

in case 4 fails (prettier) there is a script name prettier:format that can fix that automatically and let the dev add the changes to their commit

NEXT STEP

Setup semaphore 2 and make this run also on the CI.

.eslintrc Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
.nvmrc Outdated Show resolved Hide resolved
.prettierrc.yml Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
@import "~tokens/tokens.scss";

.story-body {
padding: $space*2;
padding: $space * 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @mikrotron 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:suspect:

package.json Outdated Show resolved Hide resolved
.eslintrc Outdated
"rules": {
"import/no-named-default": "off",
"camelcase": "off",
"import/no-extraneous-dependencies": "off",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a useful rule, when configured properly. I updated eslint-plugin-import to the latest version and set up the packageDir option. Was able to find a few missing dependencies in the packages.

Copy link
Contributor Author

@nahumzs nahumzs Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure how to fix this one, for stories complains that @storybook/react doesn't exist. Maybe you can take a stab and organizes and clean all this config later :D ...
screen shot 2019-02-11 at 9 36 49 am

.eslintrc Outdated Show resolved Hide resolved
"es6": true,
"browser": true,
"node": true,
"jest": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to restrict jest and cypress to just the files they're relevant for. Can be done later

.eslintrc Outdated
"import/no-unresolved": "off",
"jsx-a11y/label-has-for": "off",
"jsx-a11y/href-no-hash": "off",
"array-callback-return": "off",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems fine too... let's revisit later

Alex Zherdev and others added 6 commits February 5, 2019 09:29
Co-Authored-By: nahumzs <nahumzs@users.noreply.github.com>
Co-Authored-By: nahumzs <nahumzs@users.noreply.github.com>
Co-Authored-By: nahumzs <nahumzs@users.noreply.github.com>
@@ -46,7 +46,11 @@ storiesOf("Stylers", module).add("Include Examples", () => (
<h4>
<code>stylers.visuallyHidden</code>
</h4>
<InvisibleBox>👻</InvisibleBox>
<InvisibleBox>
<span role="img" aria-label="ghost">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to appease the linter?

It's just storybook code, so it's fine, but my understanding was that unicode emojis are fairly accessible on their own. The screen reader should describe them, literally. This approach makes sense when the icon represents an idea or metaphor, but in this case the experience is probably diminished for any non-anglo users (at least if their OS is not in English).

Not an argument to change it, just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeap, a11y-linter, complains about it, I just want to make it happy :D ...

@@ -0,0 +1 @@
v10.0.0
Copy link
Contributor

@alexzherdev alexzherdev Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the v should be omitted but will probably work as is. Also I'll try to find a way not to force an exact version (i.e. if I already have 10.15 installed, ideally nvm use would just use that)

Not blocking now.

ETA: found it. You can just say 10 and it will match any 10.x version you already have installed (it will pick the latest)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executing node -v > .nvmrc, they added the v automatically, maybe does not need it but best practices. no sure...

@nahumzs nahumzs merged commit 6e5b339 into master Feb 12, 2019
@nahumzs nahumzs deleted the UX-431-static-testing branch February 12, 2019 00:32
@nahumzs nahumzs mentioned this pull request Feb 14, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup paprika repository
3 participants